Implementation Plan

Analysis

The issue has three interconnected features: (1) tags on BuildInput, (2) discovery/store improvements, (3) interactive TUI. These build on each other — tags and discovery fixes are prerequisites for meaningful search and browsing.

Critical technical finding: BuildInput.hash uses self.model_dump_json() directly. Adding any field — even tags: None — changes the JSON output and breaks all existing hashes. The fix: self.model_dump_json(exclude={"tags"}) in the hash property. Since None fields are included by default in Pydantic v2, this exclusion is mandatory even when tags are unset.

The CSV dot-notation in prepare.py already handles arbitrary nesting via reduce(setdefault) — tags.formulation_id will become {"tags": {"formulation_id": "value"}} automatically once BuildInput accepts the field.

Deliverables

Stage 1: Tags + Discovery Fixes

• BuildInput gains tags: dict[str, str] | None = None field, excluded from hash
• SimulationStore gains min_status parameter (constructor + discover)
• Tags round-trip through CSV dot-notation and YAML
• Tags surfaced in build_metadata_table() output (merged into base columns alongside hash/path)

Stage 2: CLI Search Command

• SimulationStore.search() method with tag, type, status, SMILES, hash filters
• SMILES matching via RDKit substructure search (graceful fallback if RDKit unavailable)
• mdfactory search CLI subcommand calling store search method
• Tabular output (Rich table) with hash, type, status, tags, path

Stage 3: Textual TUI

• New [tui] optional extra with textual dependency
• mdfactory browse command launches interactive browser
• DataTable with live filtering — calls same SimulationStore.search() as CLI
• Schema-adaptive filter panels based on simulation_type
• Per-simulation detail view

Acceptance Criteria

1. BuildInput(tags={"project": "foo"}).hash == BuildInput(tags=None).hash == BuildInput().hash for same chemistry
2. Existing simulations (no tags in YAML) load without error; build_input.tags is None
3. CSV with tags.formulation_id column produces BuildInput with tags={"formulation_id": "value"}
4. SimulationStore(roots=[...], min_status="build") discovers build-only sims
5. build_metadata_table() output includes tag columns for simulations that have tags
6. mdfactory search --tag project=foo --type bilayer filters correctly
7. mdfactory search --smiles "CCO" finds sims containing ethanol (substructure match)
8. mdfactory browse launches Textual TUI with working filters
9. All new code has tests; existing tests still pass

Files to Modify

Stage 1:

Stage 2:

Stage 3:

Design Principles

• Single filter implementation: SimulationStore.search() is the one filtering API. Both CLI and TUI call it — no duplicate filter logic.
• Reuse existing infra: Tags merge into build_metadata_table() loop (no separate method); SMILES utility lives in chemistry_utilities.py (already has RDKit); CLI follows existing cyclopts sub-app pattern.
• Graceful degradation: SMILES matching checks RDKit availability at call time (conda-only dep); TUI checks textual availability. Clear error messages when missing.

Testing Approach

• Hash stability: Parametrize test with same chemistry +/- tags; assert identical hash
• Backward compat: Load YAML without tags key; assert tags is None, hash unchanged
• CSV tags: DataFrame with tags.x columns; assert BuildInput.tags == {"x": value}
• Store min_status: Fixtures with build-only sims; assert discovered with min_status="build"
• Search method: Unit test filter predicates independently; combination tests
• SMILES matching: Substructure match (ethanol in propanol), non-match, RDKit-unavailable path
• CLI search: Mock store; test output formatting and flag parsing
• TUI: textual.testing.App.run_test() for headless filter/table behavior

Risks and Open Questions

1. Tag value types: Using dict[str, str] per issue spec. Numeric CSV values stringified. Upgradable later.
2. SMILES matching: Starting with substructure (HasSubstructMatch) — most chemically useful. Tanimoto similarity deferred.
3. RDKit availability: Conda-only. Search/TUI gracefully degrade with clear error when RDKit missing.
4. Discovery caching: Deferred to follow-up issue — performance optimization, not correctness.
5. TUI scope: Read-only (browse/filter). Actions (run analysis, submit) deferred.
6. Tag immutability: Tags are set at build time. Updating tags post-build is a future enhancement.

Plan created by mach6

Progress Update

Implementation of issue 23 — simulation search, tagging & TUI browser:

Delivered

• Tags on BuildInput: tags: dict[str, str] | None field, excluded from hash via model_dump_json(exclude={"tags"}) for stability
• Store improvements: min_status param on discover(), search() method with 5 filter predicates (type, status, hash_prefix, tags, smiles substructure)
• Chemistry utilities: smiles_substructure_match() with RDKit logger suppression
• CLI: mdfactory search command with Rich table output and all filter flags
• TUI: mdfactory browse interactive browser with live-filtering DataTable, Select widgets for type/status, Input for hash/tags/SMILES, detail panel with full composition info
• Tests: 68 targeted tests covering models, store, search, CLI, TUI, and chemistry utilities

Not included

• Molecule structure viewer (deferred — terminal image rendering over SSH was insufficient)

Commit: 5c0786b

Progress tracked by mach6

Code Review

Critical

No critical findings.

Important

Finding 1 — Infinite poll loop when a Parsl future never completes (error-auditor, confidence: 85)
mdfactory/orchestration/build.py — The while True poll loop exits only when done_count == total. If a Parsl future never transitions to done() (e.g. SLURM job OOM-killed before Parsl detects it, or Parsl's monitoring thread crashes), the loop spins forever. User sees progress frozen with no error. Ctrl+C works, but there's no automatic timeout/give-up path.

Finding 2 — Schema-specific filters not implemented (completeness-checker, confidence: 95)
Neither CLI mdfactory search nor the TUI exposes per-type filter fields (e.g. z_padding for bilayer, target_density for mixedbox). The --type flag only filters by simulation_type string. Issue AC7 explicitly requires adaptive schema-specific filter panels.

Finding 3 — SimulationStore.search(smiles=...) is completely untested (test-reviewer, confidence: 100)
The SMILES substructure filter (iterates species, calls smiles_substructure_match, guards missing RDKit) has zero test coverage. test_chemistry_search.py covers the utility in isolation, but the integration path through search() is never exercised.

Finding 4 — SimulationStore.search(status=...) filter is completely untested (test-reviewer, confidence: 100)
The status parameter filter uses STATUS_ORDER index arithmetic. No test controls .status on mock Simulation objects to verify the threshold logic.

Finding 5 — Chemistry extractor functions and SimulationStore convenience methods are untested (test-reviewer, confidence: 100)
Six public SimulationStore methods (build_lnp_chemistry_table, build_all_species_table, build_chemistry_table, load_analysis_with_lnp_chemistry, load_analysis_with_all_species, load_analysis_with_chemistry) and three extractor functions (extract_all_species, make_chemistry_extractor, get_chemistry_extractor) have zero tests.

Suggestions

Finding 6 — @work(thread=True) without exclusive=True causes stale-results race (code-reviewer, confidence: 90)
mdfactory/tui/app.py line 242 — Every keystroke launches a new background worker. If a slow search finishes after a fast one, it overwrites the cache with stale results. Fix: @work(thread=True, exclusive=True).

Finding 7 — CLI search doesn't catch ValueError for invalid SMILES (code-reviewer, confidence: 85)
mdfactory/cli.py — store.search(smiles=smiles) raises ValueError on invalid SMILES, producing a raw traceback. The TUI correctly catches this with except (ValueError, ImportError).

Finding 8 — Preprocessing subprocess has no timeout (error-auditor, confidence: 90)
mdfactory/cli.py analysis_preprocess — subprocess.run(cmd, ...) has no timeout. A stuck script blocks all subsequent preprocessing sequentially with no diagnostic output.

Finding 9 — _scancel_jobs swallows unexpected failures at DEBUG level (error-auditor, confidence: 82)
mdfactory/orchestration/session.py — Both non-zero scancel return codes and unexpected exceptions (PermissionError, OSError) are logged at DEBUG. SLURM jobs can remain running after Python exits with no visible indication.

Finding 10 — Documentation and examples for tagging workflow absent (completeness-checker, confidence: 95)
Issue AC8 requires documentation. No docs cover tags.key CSV notation, mdfactory search, or mdfactory browse.

Finding 11 — TUI _run_search silently drops malformed tag tokens (test-reviewer, confidence: 90)
mdfactory/tui/app.py — Tag tokens without = are silently ignored (no filter applied, no user feedback). CLI raises SystemExit(1) for the same input. Behavior divergence is untested.

Finding 12 — SimulationDetail.update_detail() never tested with real data (test-reviewer, confidence: 85)
The detail panel rendering path (rich formatting, species iteration, try/except) is completely unexercised in tests.

Finding 13 — Three-way duplicate file-output sink in CLI pull commands (simplifier, confidence: 95)
mdfactory/cli.py — Identical 8-line JSON/CSV write block appears in sync_pull_systems, sync_pull_analysis, sync_pull_artifacts. Extract to _write_df_to_file() helper.

Finding 14 — Two-way duplicate SLURM config resolution block (simplifier, confidence: 93)
mdfactory/cli.py — Identical 30-line autodiscovery-or-manual SLURM config block appears in analysis_run and analysis_artifacts_run. Extract to _resolve_slurm_config().

Finding 15 — TOCTOU race on _results_cache in TUI detail panel (error-auditor, confidence: 82)
mdfactory/tui/app.py — Background thread replaces self._results_cache while main thread reads len() + index. Fix: capture cache = self._results_cache into a local before the check.

Finding 16 — Select.BLANK guard is dead code (code-reviewer, confidence: 90)
mdfactory/tui/app.py — allow_blank=False + option values mean is not Select.BLANK is always True. The else branch is unreachable.

Finding 17 — Imports inside per-row loop in search() (code-reviewer, confidence: 90)
mdfactory/analysis/store.py — STATUS_ORDER and smiles_substructure_match imports are inside the loop. The ImportError for RDKit is raised mid-iteration rather than fail-fast before the loop.

Finding 18 — remove_all_analyses(simulation_type=...) filter path untested (test-reviewer, confidence: 90)
The simulation_type filter param is tested for remove_all_artifacts but not for the analogous remove_all_analyses.

Finding 19 — Additional simplification opportunities in CLI (simplifier, confidence: 85–92)
Push-results logging (2 sites), show-and-confirm removal pattern (2 sites), parallel-dispatch block (2 sites), and tags_str formatting (3 sites across 2 files) are all duplicated.

Strengths

• Clean architecture: Single SimulationStore.search() method serves both CLI and TUI — no duplicated filter logic
• Robust Parsl lifecycle: parsl_session context manager with proper DFK guard, cleanup, and scancel on exit
• Comprehensive cluster autodiscovery: Well-structured parsing of sinfo/sacctmgr with good error handling
• Hash stability: Tags correctly excluded from hash computation; backward-compatible with existing simulations
• Strong test count: 68+ new tests covering models, store, CLI, TUI, orchestration, and cluster
• Good lazy-import pattern: Optional deps (Parsl, Textual, RDKit) gracefully degrade with clear messages

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

Reviewed by mach6

Review Assessment

Review comment

Classifications

Action Plan

1. Fix @work(thread=True) race in TUI — add exclusive=True to _run_search (finding 6, also resolves finding 15)
2. Hoist imports out of per-row loop in search() — fail-fast on missing RDKit (finding 17)
3. Add ValueError handling in CLI search command for invalid SMILES (finding 7)
4. Remove dead Select.BLANK guards in TUI (finding 16)
5. Add tests for search(smiles=...) integration (finding 3)
6. Add tests for search(status=...) threshold logic (finding 4)
7. Add tests for chemistry extractors (finding 5)
8. Extract duplicate file-output helper in CLI (finding 13)
9. Extract duplicate SLURM config resolution in CLI (finding 14)
10. Add timeout/stall detection to Parsl poll loop (finding 1)
11. Add test for remove_all_analyses(simulation_type=...) filter (finding 18)

Deferred (track separately):

• Finding 2: Schema-specific filters (AC7) — separate issue needed
• Finding 10: Documentation for tagging workflow (AC8) — separate issue needed

Assessment by mach6

Progress Update

Addressed 11 review findings from the mach6-review:

Bug fixes

• Finding 1: Added stall_timeout (600s default) to Parsl poll loop — warns when no futures complete for extended period
• Finding 6: Added exclusive=True to TUI @work(thread=True) — prevents stale search results race
• Finding 7: Added try/except (ValueError, ImportError) in CLI search command for invalid SMILES
• Finding 16: Removed dead Select.BLANK guards in TUI (always True due to allow_blank=False)
• Finding 17: Hoisted STATUS_ORDER and smiles_substructure_match imports above the per-row loop for fail-fast behavior

Refactoring

• Finding 13: Extracted _write_output() helper — replaced 3 identical file-output blocks in CLI
• Finding 14: Extracted _resolve_slurm_config() helper — replaced 2 identical SLURM config blocks in CLI

Tests added (19 new tests)

• Finding 3: 4 tests for search(smiles=...) integration (match, no-match, filtering, ImportError)
• Finding 4: 3 tests for search(status=...) threshold logic (includes, excludes, invalid raises)
• Finding 5: 11 tests for chemistry extractors (extract_all_species, get_chemistry_extractor, build_all_species_table, build_chemistry_table) in new test_chemistry_extractors.py
• Finding 18: 1 test for remove_all_analyses(simulation_type=...) filter path

Commit: 8437382

Progress tracked by mach6